Codecov Report

Attention: Patch coverage is 1.20000% with 247 lines in your changes missing coverage. Please review.

Project coverage is 21.59%. Comparing base (d48436d) to head (b1aefc7).

@@                    Coverage Diff                     @@
##           express-lane-timeboost    #2860      +/-   ##
==========================================================
+ Coverage                   21.55%   21.59%   +0.03%     
==========================================================
  Files                         286      287       +1     
  Lines                       42546    42462      -84     
==========================================================
- Hits                         9170     9168       -2     
+ Misses                      31882    31803      -79     
+ Partials                     1494     1491       -3     

Here's my analysis where I looked for any concurrency issues.

We have a ticker that goes off (roughly) when the round starts. It deletes the previous round's roundControl entry, that round no longer has a controller.

We have a ticker that goes off at max block speed.

• On AuctionResolved events it stores a new round->controller address in roundControl
• On SetExpressLaneController (transfer) events it
  • stores a new round->controller address in roundControl, overwriting the previous
  • locks roundInfoMutex, sets a new expressLaneRoundInfo for this round, unlocks roundInfoMutex

We have txs coming in at random times.

• They are first Validated which checks the message's sender against roundControl for the current round.
• Then they are Sequenced which
  • locks the roundInfoMutex
  • checks msg's sender against roundControl again
  • various reads/writes of current round's roundInfo as messages are put into roundInfo queue, removed as they are queued with sequencer, sequence number updated
  • release roundInfoMutex

Considering some scenarios:
If we're in the middle of sequencing an express lane tx then round control is transferred, the round info (which contains the seq number and msg queue) is not reset until that tx is queued. I don't think this makes the unavoidable race between when we see the transfer event and when we see the txs any worse. We should make sure the max block speed ticker makes the most sense to get the events with minimal latency to minimize this issue.

The goroutine running the ELS new round ticker could be late to be scheduled and generate a new tick slightly late. This shouldn't be a problem because the deletion is only needed to avoid unbounded growth. A tx that comes in during this time (eg new round has started but tick hasn't occurred yet) would be rejected at validation due to re-deriving the current round from the system time.

If we are in the middle of handling a SetExpressLaneController event and get an express lane tx from the old controller, if the handling of SetExpressLaneController already changed the roundControl then the tx will be rejected. If not, then if SequenceExpressLaneSubmission gets the roundInfoMutex first it may be queued with the sequencer. If the SetExpressLaneController handling gets the mutex first, then when SequenceExpressLaneSubmission gets the mutex, it rechecks roundControl which would reject the tx.

So in summary I think there is inherent raciness around SetExpressLaneController (transfer) events and express lane txs, which at the moment I think is unavoidable. We may still process messages from the old controller or not process messages from the new controller around the moment of transfer for a brief time after the actual transfer occurs on chain, due to delays in reading the events and also not interrupting processing a message upon receiving an event. But aside from this, I don't think the event and tx happening concurrently can cause any invalid state.